Skip to content

Conversation

@Yeaseen
Copy link
Contributor

@Yeaseen Yeaseen commented Dec 18, 2025

This is a fresh version of PR #56934

Update fs.rmSync to properly handle file paths that include non-ASCII characters. This change prevents crashes and errors when attempting to delete files with international or special characters in their names.

Add a test in test/parallel to ensure that files with non-ASCII characters can be deleted without issues. This covers cases that previously caused unexpected behavior or crashes on certain file systems.

Fixes: #56049

If this gets merged, I will be working on the additional errors part.

@joyeecheung Please review this. Thank you!

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Dec 18, 2025
@Yeaseen Yeaseen force-pushed the fix-non-ascii-filepath-issue branch 2 times, most recently from 0b11867 to f2996d1 Compare December 18, 2025 15:56
@Yeaseen Yeaseen force-pushed the fix-non-ascii-filepath-issue branch from f2996d1 to 475580e Compare December 18, 2025 20:26
@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.53%. Comparing base (4f24aff) to head (daa7753).
⚠️ Report is 62 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #61108   +/-   ##
=======================================
  Coverage   88.53%   88.53%           
=======================================
  Files         703      703           
  Lines      208546   208545    -1     
  Branches    40217    40221    +4     
=======================================
+ Hits       184634   184642    +8     
+ Misses      15926    15913   -13     
- Partials     7986     7990    +4     
Files with missing lines Coverage Δ
src/node_file.cc 75.58% <50.00%> (+0.02%) ⬆️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Update fs.rmSync to properly handle file paths that include
non-ASCII characters. This change prevents crashes and errors
when attempting to delete files with international or special
characters in their names.

Add a test in test/parallel to ensure that files with
non-ASCII characters can be deleted without issues.
This covers cases that previously caused unexpected behavior
or crashes on certain file systems.

Fixes: nodejs#56049
@Yeaseen Yeaseen force-pushed the fix-non-ascii-filepath-issue branch from 475580e to daa7753 Compare December 19, 2025 00:27
Copy link
Member

@jazelly jazelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jazelly jazelly added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2025
@nodejs-github-bot
Copy link
Collaborator

@Yeaseen
Copy link
Contributor Author

Yeaseen commented Dec 20, 2025

@jazelly I can’t access Jenkins logs. Can you paste the failing test name/excerpt from node-test-commit-linuxone-rhel8-s390x or node-test-commit-aix and the other test-commit failures here?

@jazelly
Copy link
Member

jazelly commented Dec 20, 2025

hmm seems like I lost access to jenkins

@nodejs-github-bot
Copy link
Collaborator

@Yeaseen
Copy link
Contributor Author

Yeaseen commented Dec 21, 2025

@joyeecheung In the previous run, node-test-linux-linked-openssl111fips passed, but now it says failed. Could you check this, as I don't have access to the log? Thank you.

@nodejs-github-bot
Copy link
Collaborator

@avivkeller avivkeller added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 28, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2025
@nodejs-github-bot nodejs-github-bot merged commit 05d6b9b into nodejs:main Dec 28, 2025
70 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 05d6b9b

@Renegade334 Renegade334 added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Dec 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs.rmSync('速') crash without throw

9 participants